-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump Rust toolchain to 1.57, fix lints #1642
Conversation
The clippy lint has a point: the enum may be too large to be efficiently sent over a channel.
There's been a layering violation, the config module should use its own smaller domain error. This gets rid of a clippy lint, too.
They don't seem to be used for anything, and this triggers a clippy lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mikhail!
Glad to finally see the clippy fixes!
pub fn new(cfg: GlobalConfig) -> Result<Self, FrameworkError> { | ||
let filter = build_tracing_filter(cfg.log_level.to_string())?; | ||
|
||
// Construct a tracing subscriber with the supplied filter and enable reloading. | ||
let builder = FmtSubscriber::builder() | ||
.with_target(false) | ||
.with_env_filter(filter) | ||
.with_writer(std::io::stderr as StdWriter) | ||
.with_writer(std::io::stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: This code should be tested at a basic level in the E2E python scripts, which rely on the JSON output. It would still be useful to run some CLI commands with and without --json
to make sure we're not changing the structure of the output here.
We can do that, however, in the release PR (which will close #1648): on that occasion, we also would like to update the guide, which requires re-running the CLI anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a stylistic change, the cast did nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was required on a previous version of rustc, glad to see it's not needed anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah hm, this means we've implicitly bumped our MSRV.
If we care about such things, it may make sense to add a clippy.toml
and maintain the MSRV there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's do that!
* Bump Rust toolchain to 1.57 * Fix trivial lints introduced in clippy 0.1.57 * Box the value in SupervisorCmd::UpdateConfig The clippy lint has a point: the enum may be too large to be efficiently sent over a channel. * Factor config errors out into a smaller error type There's been a layering violation, the config module should use its own smaller domain error. This gets rid of a clippy lint, too. * Remove reload handles for tracing subscribers They don't seem to be used for anything, and this triggers a clippy lint. * relayer-cli: Remove trivial casts in components
Description
New rust release, new lints to fix.
Some of them have uncovered little problems in the code, which are fixed at the root cause.
PR author checklist:
Added changelog entry, usingunclog
.Added tests: integration (for Hermes) or unit/mock tests (for modules).Updated code comments and documentation (e.g.,docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.